Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(Wizard - next): supporting component unit tests #7731

Merged
merged 2 commits into from
Sep 19, 2022
Merged

feat(Wizard - next): supporting component unit tests #7731

merged 2 commits into from
Sep 19, 2022

Conversation

jpuzz0
Copy link
Contributor

@jpuzz0 jpuzz0 commented Jul 21, 2022 •
edited
Loading

What: Closes #7737

@jpuzz0 jpuzz0 added the testing label Jul 21, 2022
@jpuzz0 jpuzz0 marked this pull request as draft July 21, 2022 19:52
@patternfly-build
Copy link
Contributor

patternfly-build commented Jul 21, 2022 •
edited
Loading

Preview: https://patternfly-react-pr-7731.surge.sh

A11y report: https://patternfly-react-pr-7731-a11y.surge.sh

@jpuzz0 jpuzz0 removed the testing label Jul 25, 2022
@jpuzz0 jpuzz0 force-pushed the feature/composable-wizard-more-tests branch from 057f828 to 8eb62e9 Compare August 24, 2022 00:21
@jpuzz0 jpuzz0 marked this pull request as ready for review August 24, 2022 00:21
@jpuzz0 jpuzz0 changed the title feat(WizardComposable): supporting component unit tests feat(Wizard): supporting component unit tests Aug 24, 2022
@jpuzz0 jpuzz0 force-pushed the feature/composable-wizard-more-tests branch from 8eb62e9 to 8f07d76 Compare August 24, 2022 00:50
@jpuzz0 jpuzz0 changed the title feat(Wizard): supporting component unit tests feat(Wizard - next): supporting component unit tests Sep 1, 2022
@jpuzz0 jpuzz0 mentioned this pull request Sep 1, 2022
Closed
@jpuzz0 jpuzz0 force-pushed the feature/composable-wizard-more-tests branch from 8f07d76 to 3eb5f6c Compare September 1, 2022 13:51
@jpuzz0 jpuzz0 mentioned this pull request Sep 1, 2022
Merged
nicolethoen
nicolethoen reviewed Sep 2, 2022
View reviewed changes
packages/react-core/src/next/components/Wizard/WizardToggle.tsx Outdated
/** Navigate using the step index */
goToStepByIndex: (index: number) => void;
/** The button's aria-label */
/** Custom WizardNav or callback used to create a default WizardNav */
nav?: DefaultWizardNavProps | CustomWizardNavFunction;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible/wise to have the nav and the footer both use the same pattern? Right now, the custom footer is a props object or a react node and the nav is a props object or a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's necessary, but we could make a CustomWizardFooterFunction if you want that returns onNext, onBack, and onClose. Thoughts on that change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That'd be helpful if people want a custom footer with custom buttons, right? so maybe that'd be wise

Copy link
Contributor Author

@jpuzz0 jpuzz0 Sep 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. That will make things complicated with how the footer is managed in context. We'd end up initializing footer's state variable as a function, which I'm not sure is even possible. Maybe I can try to do this in the next PR here where I'm updating a bunch of types (and including a new one called CustomWizardNavItemFunction - very similar to CustomWizardNavFunction):
https://github.com/patternfly/patternfly-react/pull/7915/files#diff-087f56845137e426d26fe8b189b155902a59de75ee7e2beff5c07db4d4f9c25f

Unless you think this is a blocking change for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's blocking since all this is beta :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok cool. I will address it in that other PR then, thanks!

wise-king-sullyman
wise-king-sullyman reviewed Sep 2, 2022
View reviewed changes
Copy link
Contributor

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I won't have time to review this in depth until I return from PTO on Friday 9/9, but one thing that I know will need to change is that the userEvent usage will need to be updated to be compatible with user-event v14+

@jpuzz0 jpuzz0 force-pushed the feature/composable-wizard-more-tests branch from 3eb5f6c to 0007e01 Compare September 6, 2022 14:22
tlabaj
tlabaj approved these changes Sep 7, 2022
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jpuzz0
Copy link
Contributor Author

jpuzz0 commented Sep 8, 2022

Unfortunately I won't have time to review this in depth until I return from PTO on Friday 9/9, but one thing that I know will need to change is that the userEvent usage will need to be updated to be compatible with user-event v14+

This has been addressed.

wise-king-sullyman
wise-king-sullyman reviewed Sep 9, 2022
View reviewed changes
Copy link
Contributor

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got a couple of nits and a couple of questions about places where I think it would be better to test behavior on a sub-component directly.

packages/react-core/src/next/components/Wizard/__tests__/Wizard.test.tsx Outdated Show resolved Hide resolved
packages/react-core/src/next/components/Wizard/__tests__/WizardBody.test.tsx Outdated Show resolved Hide resolved
packages/react-core/src/next/components/Wizard/__tests__/WizardToggle.test.tsx Outdated Show resolved Hide resolved
packages/react-core/src/next/components/Wizard/__tests__/WizardToggle.test.tsx Outdated Show resolved Hide resolved
wise-king-sullyman
wise-king-sullyman approved these changes Sep 14, 2022
View reviewed changes
Copy link
Contributor

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@jpuzz0 jpuzz0 force-pushed the feature/composable-wizard-more-tests branch from 20a54bd to b36baeb Compare September 19, 2022 14:51
@tlabaj tlabaj merged commit 2e4ec26 into patternfly:main Sep 19, 2022
@patternfly-build
Copy link
Contributor

Your changes have been released in:

  • eslint-plugin-patternfly-react@4.80.0
  • @patternfly/react-catalog-view-extension@4.92.0
  • @patternfly/react-charts@6.94.0
  • @patternfly/react-code-editor@4.82.0
  • @patternfly/react-console@4.92.0
  • @patternfly/react-core@4.241.0
  • @patternfly/react-docs@5.102.0
  • @patternfly/react-icons@4.92.0
  • @patternfly/react-inline-edit-extension@4.86.0
  • demo-app-ts@4.201.0
  • @patternfly/react-integration@4.203.0
  • @patternfly/react-log-viewer@4.86.0
  • @patternfly/react-styles@4.91.0
  • @patternfly/react-table@4.110.0
  • @patternfly/react-tokens@4.93.0
  • @patternfly/react-topology@4.88.0
  • @patternfly/react-virtualized-extension@4.88.0
  • transformer-cjs-imports@4.79.0

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wise-king-sullyman wise-king-sullyman wise-king-sullyman approved these changes

@nicolethoen nicolethoen nicolethoen approved these changes

@tlabaj tlabaj Awaiting requested review from tlabaj

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat(Wizard - next): supporting component unit tests
5 participants
@jpuzz0 @patternfly-build @wise-king-sullyman @nicolethoen @tlabaj